Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generate block command #254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benthecarman
Copy link

No description provided.

@benthecarman benthecarman force-pushed the generate-block branch 3 times, most recently from 73b7912 to 5a271f2 Compare November 1, 2022 22:46
@andrewtoth
Copy link

This RPC takes bothTxids and RawTxs. We should support both I think. Perhaps by making it take a new enum that is either a RawTx or Txid?

@andrewtoth
Copy link

Also dupe of #189. Not sure if that author is still active though.

@benthecarman
Copy link
Author

This RPC takes bothTxids and RawTxs. We should support both I think. Perhaps by making it take a new enum that is either a RawTx or Txid?

done

integration_test/src/main.rs Show resolved Hide resolved
@apoelstra
Copy link
Member

Thanks @benthecarman! concept ACK from me. I have a mild preference that the enum have a more generateblock-specific name but I can't think of a good one.

Heads up that this does not compile on Rust 1.41, which is our MSRV. I've approved CI so you should see a failure soon but here is the error on my end:

error[E0277]: the trait bound `bitcoin::blockdata::transaction::Transaction: bitcoincore_rpc::client::RawTx` is not satisfied
   --> integration_test/src/main.rs:376:29
    |
376 |     cl.send_raw_transaction(tx).unwrap();
    |                             ^^ the trait `bitcoincore_rpc::client::RawTx` is not implemented for `bitcoin::blockdata::transaction::Transaction`
    |
    = help: the following implementations were found:
              <&'a bitcoin::blockdata::transaction::Transaction as bitcoincore_rpc::client::RawTx>

error: aborting due to previous error

/// The different authentication methods for the client.
#[derive(Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum MineableTx {
RawTx(Transaction),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this doesn't have to be a Transaction but a RawTx trait object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really work, have to make it a Box<dyn RawTx> but still run into errors with sizing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it would actually have to be a generic enum.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually maybe not...

@benthecarman
Copy link
Author

Fixed the compile issue I believe

@apoelstra
Copy link
Member

@benthecarman compilation is fixed, but I believe now this is going to fail because cargo fmt doesn't pass :)

@benthecarman
Copy link
Author

@benthecarman compilation is fixed, but I believe now this is going to fail because cargo fmt doesn't pass :)

Ah yeah there we're some unrelated to my changes so I didn't fix

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK badf7fc

@apoelstra
Copy link
Member

What version of core was this introduced in? It looks like it's not present in 0.19 and 0.20, which we run integration tests against.

I'm not sure how we handle feature-gating new RPCs here. It's not really my crate..

@andrewtoth
Copy link

Version 0.21

@tcharding
Copy link
Member

Do you have clock cycles for this PR @benthecarman? Otherwise I can pick it up, if I do it it will not get done till after next release.

FTR I'm attempting to do a quick-ish release then start working on the harder stuff (update crate to support latest Core version).

@benthecarman
Copy link
Author

I am at bitcoin++ this week, if you want a fast release go ahead and take this over

@tcharding
Copy link
Member

Ha! 'quick-ish release' I must have been on crack when I wrote that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants